lib/repo: Enable locking by default, but drop external API

The code has been sitting around for a while but since I disabled
it by default, I doubt anyone is really using it or relying on it.

This patch and turns on locking by default, and also drops the
API which was only public in the experimental API builds.
Conceptually these are two distinct things, and we
may actually want to split up the patches.

I don't think this will break anyone, but it's hard to say for sure.
It's also going to be hard to find out until we actually release
I suspect...

But anyone who is broken should be able to add `locking=false` into
their repo config.  On the flip side Endless has been shipping with
this enabled and it is reported to help.

The reason to drop the APIs: I'm a bit concerned about the interactions over time
between libostree's use of the API and any apps that start using it.
For example, if an app specifies a SHARED lock in their code, then
later internally we decide to temporarily grab an `EXCLUSIVE`, but the
app had a second thread/process that was `EXCLUSIVE` already, and
that process was waiting on the first bit of code, then we could
deadlock. I can't think of a real world situation where this would happen
yet though.

We are likely to in the future have say `fsck` take an external lock,
`checkout` grab a shared one, etc.

Closes: #1555
Approved by: jlebon

diff --git a/apidoc/ostree-experimental-sections.txt b/apidoc/ostree-experimental-sections.txt
index 0d1684066929e8f32d9b1ad5604fc55e9f9589df..60daaca5096ac076f273bb5d87ee2dfc25aced0d 100644 (file)
--- a/apidoc/ostree-experimental-sections.txt
+++ b/apidoc/ostree-experimental-sections.txt
@@ -90,12 +90,6 @@ ostree_repo_finder_override_get_type
 
 <SECTION>
 <FILE>ostree-misc-experimental</FILE>
-OstreeRepoLockType
-ostree_repo_lock_push
-ostree_repo_lock_pop
-OstreeRepoAutoLock
-ostree_repo_auto_lock_push
-ostree_repo_auto_lock_cleanup
 ostree_repo_get_collection_id
 ostree_repo_set_collection_id
 ostree_validate_collection_id

diff --git a/src/libostree/libostree-experimental.sym b/src/libostree/libostree-experimental.sym
index 3f3454f3bcd22b5ea4a86d869bb05a70909c9b6e..b83ad1b0d2312bda5ed7fbd7b2118b8e787c159e 100644 (file)
--- a/src/libostree/libostree-experimental.sym
+++ b/src/libostree/libostree-experimental.sym
@@ -94,8 +94,4 @@ LIBOSTREE_2017.14_EXPERIMENTAL {
 global:
   ostree_remote_get_type;
   ostree_remote_get_url;
-  ostree_repo_auto_lock_cleanup;
-  ostree_repo_auto_lock_push;
-  ostree_repo_lock_pop;
-  ostree_repo_lock_push;
 } LIBOSTREE_2017.13_EXPERIMENTAL;

diff --git a/src/libostree/ostree-autocleanups.h b/src/libostree/ostree-autocleanups.h
index 75b498fccdada2b6bde3461ec7e83b21ce14389f..504954e00fb2a931a08dfdfdac4e5882981bb494 100644 (file)
--- a/src/libostree/ostree-autocleanups.h
+++ b/src/libostree/ostree-autocleanups.h
@@ -61,7 +61,6 @@ G_DEFINE_AUTOPTR_CLEANUP_FUNC (OstreeSysrootUpgrader, g_object_unref)
 G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC (OstreeRepoCommitTraverseIter, ostree_repo_commit_traverse_iter_clear)
 
 #ifdef OSTREE_ENABLE_EXPERIMENTAL_API
-G_DEFINE_AUTOPTR_CLEANUP_FUNC (OstreeRepoAutoLock, ostree_repo_auto_lock_cleanup)
 G_DEFINE_AUTOPTR_CLEANUP_FUNC (OstreeCollectionRef, ostree_collection_ref_free)
 G_DEFINE_AUTO_CLEANUP_FREE_FUNC (OstreeCollectionRefv, ostree_collection_ref_freev, NULL)
 G_DEFINE_AUTOPTR_CLEANUP_FUNC (OstreeRemote, ostree_remote_unref)

diff --git a/src/libostree/ostree-repo-commit.c b/src/libostree/ostree-repo-commit.c
index c171b3da3ec2a119b2b104129cd55e1b6b25c20d..6eb645bedb5fdfa80a363408a636472cfe0c31d8 100644 (file)
--- a/src/libostree/ostree-repo-commit.c
+++ b/src/libostree/ostree-repo-commit.c
@@ -1567,8 +1567,8 @@ ostree_repo_prepare_transaction (OstreeRepo     *self,
 
   memset (&self->txn.stats, 0, sizeof (OstreeRepoTransactionStats));
 
-  self->txn_locked = ostree_repo_lock_push (self, OSTREE_REPO_LOCK_SHARED,
-                                            cancellable, error);
+  self->txn_locked = _ostree_repo_lock_push (self, OSTREE_REPO_LOCK_SHARED,
+                                             cancellable, error);
   if (!self->txn_locked)
     return FALSE;
 
@@ -2136,7 +2136,7 @@ ostree_repo_commit_transaction (OstreeRepo                  *self,
 
   if (self->txn_locked)
     {
-      if (!ostree_repo_lock_pop (self, cancellable, error))
+      if (!_ostree_repo_lock_pop (self, cancellable, error))
         return FALSE;
       self->txn_locked = FALSE;
     }
@@ -2189,7 +2189,7 @@ ostree_repo_abort_transaction (OstreeRepo     *self,
 
   if (self->txn_locked)
     {
-      if (!ostree_repo_lock_pop (self, cancellable, error))
+      if (!_ostree_repo_lock_pop (self, cancellable, error))
         return FALSE;
       self->txn_locked = FALSE;
     }

diff --git a/src/libostree/ostree-repo-private.h b/src/libostree/ostree-repo-private.h
index 3078a9e29369664a2fd3ece99495449b346e286c..77203638e863be8a9ae175684591d2f650d7afbe 100644 (file)
--- a/src/libostree/ostree-repo-private.h
+++ b/src/libostree/ostree-repo-private.h
@@ -435,34 +435,36 @@ _ostree_repo_get_remote_inherited (OstreeRepo  *self,
                                    const char  *name,
                                    GError     **error);
 
-#ifndef OSTREE_ENABLE_EXPERIMENTAL_API
-
-/* All the locking APIs below are duplicated in ostree-repo.h. Remove the ones
- * here once it's no longer experimental.
+/* Locking APIs are currently private.
+ * See https://github.com/ostreedev/ostree/pull/1555
  */
-
 typedef enum {
   OSTREE_REPO_LOCK_SHARED,
   OSTREE_REPO_LOCK_EXCLUSIVE
 } OstreeRepoLockType;
 
-gboolean      ostree_repo_lock_push (OstreeRepo          *self,
+gboolean      _ostree_repo_lock_push (OstreeRepo          *self,
                                      OstreeRepoLockType   lock_type,
                                      GCancellable        *cancellable,
                                      GError             **error);
-gboolean      ostree_repo_lock_pop (OstreeRepo    *self,
-                                    GCancellable  *cancellable,
-                                    GError       **error);
+gboolean      _ostree_repo_lock_pop (OstreeRepo    *self,
+                                     GCancellable  *cancellable,
+                                     GError       **error);
 
 typedef OstreeRepo OstreeRepoAutoLock;
 
-OstreeRepoAutoLock * ostree_repo_auto_lock_push (OstreeRepo          *self,
-                                                 OstreeRepoLockType   lock_type,
-                                                 GCancellable        *cancellable,
-                                                 GError             **error);
-void          ostree_repo_auto_lock_cleanup (OstreeRepoAutoLock *lock);
-G_DEFINE_AUTOPTR_CLEANUP_FUNC (OstreeRepoAutoLock, ostree_repo_auto_lock_cleanup)
+OstreeRepoAutoLock * _ostree_repo_auto_lock_push (OstreeRepo          *self,
+                                                  OstreeRepoLockType   lock_type,
+                                                  GCancellable        *cancellable,
+                                                  GError             **error);
+void          _ostree_repo_auto_lock_cleanup (OstreeRepoAutoLock *lock);
+G_DEFINE_AUTOPTR_CLEANUP_FUNC (OstreeRepoAutoLock, _ostree_repo_auto_lock_cleanup)
 
+#ifndef OSTREE_ENABLE_EXPERIMENTAL_API
+
+/* These APIs are duplicated in the public headers when doing an
+ * experimental-API build.
+ */
 const gchar * ostree_repo_get_collection_id (OstreeRepo   *self);
 gboolean      ostree_repo_set_collection_id (OstreeRepo   *self,
                                              const gchar  *collection_id,

diff --git a/src/libostree/ostree-repo-prune.c b/src/libostree/ostree-repo-prune.c
index f0c0a9741121b8e953f4c86893af86878234f5c0..2ffd69488937f01d8cdfc0733df259e8f3483807 100644 (file)
--- a/src/libostree/ostree-repo-prune.c
+++ b/src/libostree/ostree-repo-prune.c
@@ -201,8 +201,7 @@ ostree_repo_prune_static_deltas (OstreeRepo *self, const char *commit,
                                  GError           **error)
 {
   g_autoptr(OstreeRepoAutoLock) lock =
-    ostree_repo_auto_lock_push (self, OSTREE_REPO_LOCK_EXCLUSIVE, cancellable,
-                                error);
+    _ostree_repo_auto_lock_push (self, OSTREE_REPO_LOCK_EXCLUSIVE, cancellable, error);
   if (!lock)
     return FALSE;
 
@@ -340,8 +339,7 @@ ostree_repo_prune (OstreeRepo        *self,
                    GError           **error)
 {
   g_autoptr(OstreeRepoAutoLock) lock =
-    ostree_repo_auto_lock_push (self, OSTREE_REPO_LOCK_EXCLUSIVE, cancellable,
-                                error);
+    _ostree_repo_auto_lock_push (self, OSTREE_REPO_LOCK_EXCLUSIVE, cancellable, error);
   if (!lock)
     return FALSE;
 
@@ -452,8 +450,7 @@ ostree_repo_prune_from_reachable (OstreeRepo        *self,
                                   GError           **error)
 {
   g_autoptr(OstreeRepoAutoLock) lock =
-    ostree_repo_auto_lock_push (self, OSTREE_REPO_LOCK_EXCLUSIVE, cancellable,
-                                error);
+    _ostree_repo_auto_lock_push (self, OSTREE_REPO_LOCK_EXCLUSIVE, cancellable, error);
   if (!lock)
     return FALSE;
 

diff --git a/src/libostree/ostree-repo.c b/src/libostree/ostree-repo.c
index 7d593f506ed06fd4bb8754cdc1e5505018a4eaf0..79006a6bb3e17e093c9caa47d54a51c9f062759f 100644 (file)
--- a/src/libostree/ostree-repo.c
+++ b/src/libostree/ostree-repo.c
@@ -436,7 +436,7 @@ pop_repo_lock (OstreeRepo  *self,
   return TRUE;
 }
 
-/**
+/*
  * ostree_repo_lock_push:
  * @self: a #OstreeRepo
  * @lock_type: the type of lock to acquire
@@ -462,13 +462,12 @@ pop_repo_lock (OstreeRepo  *self,
  * %TRUE is returned.
  *
  * Returns: %TRUE on success, otherwise %FALSE with @error set
- * Since: 2017.14
  */
 gboolean
-ostree_repo_lock_push (OstreeRepo          *self,
-                       OstreeRepoLockType   lock_type,
-                       GCancellable        *cancellable,
-                       GError             **error)
+_ostree_repo_lock_push (OstreeRepo          *self,
+                        OstreeRepoLockType   lock_type,
+                        GCancellable        *cancellable,
+                        GError             **error)
 {
   g_return_val_if_fail (self != NULL, FALSE);
   g_return_val_if_fail (OSTREE_IS_REPO (self), FALSE);
@@ -531,8 +530,8 @@ ostree_repo_lock_push (OstreeRepo          *self,
     }
 }
 
-/**
- * ostree_repo_lock_pop:
+/*
+ * _ostree_repo_lock_pop:
  * @self: a #OstreeRepo
  * @cancellable: a #GCancellable
  * @error: a #GError
@@ -553,12 +552,11 @@ ostree_repo_lock_push (OstreeRepo          *self,
  * %TRUE is returned.
  *
  * Returns: %TRUE on success, otherwise %FALSE with @error set
- * Since: 2017.14
  */
 gboolean
-ostree_repo_lock_pop (OstreeRepo    *self,
-                      GCancellable  *cancellable,
-                      GError       **error)
+_ostree_repo_lock_pop (OstreeRepo    *self,
+                       GCancellable  *cancellable,
+                       GError       **error)
 {
   g_return_val_if_fail (self != NULL, FALSE);
   g_return_val_if_fail (OSTREE_IS_REPO (self), FALSE);
@@ -621,8 +619,8 @@ ostree_repo_lock_pop (OstreeRepo    *self,
     }
 }
 
-/**
- * ostree_repo_auto_lock_push: (skip)
+/*
+ * _ostree_repo_auto_lock_push: (skip)
  * @self: a #OstreeRepo
  * @lock_type: the type of lock to acquire
  * @cancellable: a #GCancellable
@@ -636,37 +634,34 @@ ostree_repo_lock_pop (OstreeRepo    *self,
  *
  * |[<!-- language="C" -->
  * g_autoptr(OstreeRepoAutoLock) lock = NULL;
- * lock = ostree_repo_auto_lock_push (repo, lock_type, cancellable, error);
+ * lock = _ostree_repo_auto_lock_push (repo, lock_type, cancellable, error);
  * if (!lock)
  *   return FALSE;
  * ]|
  *
  * Returns: @self on success, otherwise %NULL with @error set
- * Since: 2017.14
  */
 OstreeRepoAutoLock *
-ostree_repo_auto_lock_push (OstreeRepo          *self,
-                            OstreeRepoLockType   lock_type,
-                            GCancellable        *cancellable,
-                            GError             **error)
+_ostree_repo_auto_lock_push (OstreeRepo          *self,
+                             OstreeRepoLockType   lock_type,
+                             GCancellable        *cancellable,
+                             GError             **error)
 {
-  if (!ostree_repo_lock_push (self, lock_type, cancellable, error))
+  if (!_ostree_repo_lock_push (self, lock_type, cancellable, error))
     return NULL;
   return (OstreeRepoAutoLock *)self;
 }
 
-/**
- * ostree_repo_auto_lock_cleanup: (skip)
+/*
+ * _ostree_repo_auto_lock_cleanup: (skip)
  * @lock: a #OstreeRepoAutoLock
  *
  * A cleanup handler for use with ostree_repo_auto_lock_push(). If @lock is
  * not %NULL, ostree_repo_lock_pop() will be called on it. If
  * ostree_repo_lock_pop() fails, a critical warning will be emitted.
- *
- * Since: 2017.14
  */
 void
-ostree_repo_auto_lock_cleanup (OstreeRepoAutoLock *lock)
+_ostree_repo_auto_lock_cleanup (OstreeRepoAutoLock *lock)
 {
   OstreeRepo *repo = lock;
   if (repo)
@@ -674,7 +669,7 @@ ostree_repo_auto_lock_cleanup (OstreeRepoAutoLock *lock)
       g_autoptr(GError) error = NULL;
       int errsv = errno;
 
-      if (!ostree_repo_lock_pop (repo, NULL, &error))
+      if (!_ostree_repo_lock_pop (repo, NULL, &error))
         g_critical ("Cleanup repo lock failed: %s", error->message);
 
       errno = errsv;
@@ -2742,10 +2737,10 @@ reload_core_config (OstreeRepo          *self,
     self->tmp_expiry_seconds = g_ascii_strtoull (tmp_expiry_seconds, NULL, 10);
   }
 
-  /* Disable locking by default for now */
   { gboolean locking;
+    /* Enabled by default in 2018.05 */
     if (!ot_keyfile_get_boolean_with_default (self->config, "core", "locking",
-                                              FALSE, &locking, error))
+                                              TRUE, &locking, error))
       return FALSE;
     if (!locking)
       {

diff --git a/src/libostree/ostree-repo.h b/src/libostree/ostree-repo.h
index 04b044164c3b788e73116e45cc549072a04d00dd..8d3a7a6f20c2390195f240ef5a2b31ec613d70c0 100644 (file)
--- a/src/libostree/ostree-repo.h
+++ b/src/libostree/ostree-repo.h
@@ -109,48 +109,6 @@ OstreeRepo *  ostree_repo_create_at (int             dfd,
 
 #ifdef OSTREE_ENABLE_EXPERIMENTAL_API
 
-/**
- * OstreeRepoLockType:
- * @OSTREE_REPO_LOCK_SHARED: A shared lock
- * @OSTREE_REPO_LOCK_EXCLUSIVE: An exclusive lock
- *
- * The type of repository lock to acquire.
- *
- * Since: 2017.14
- */
-typedef enum {
-  OSTREE_REPO_LOCK_SHARED,
-  OSTREE_REPO_LOCK_EXCLUSIVE
-} OstreeRepoLockType;
-
-_OSTREE_PUBLIC
-gboolean      ostree_repo_lock_push (OstreeRepo          *self,
-                                     OstreeRepoLockType   lock_type,
-                                     GCancellable        *cancellable,
-                                     GError             **error);
-_OSTREE_PUBLIC
-gboolean      ostree_repo_lock_pop (OstreeRepo    *self,
-                                    GCancellable  *cancellable,
-                                    GError       **error);
-
-/**
- * OstreeRepoAutoLock: (skip)
- *
- * This is simply an alias to #OstreeRepo used for automatic lock cleanup.
- * See ostree_repo_auto_lock_push() for its intended usage.
- *
- * Since: 2017.14
- */
-typedef OstreeRepo OstreeRepoAutoLock;
-
-_OSTREE_PUBLIC
-OstreeRepoAutoLock * ostree_repo_auto_lock_push (OstreeRepo          *self,
-                                                 OstreeRepoLockType   lock_type,
-                                                 GCancellable        *cancellable,
-                                                 GError             **error);
-_OSTREE_PUBLIC
-void          ostree_repo_auto_lock_cleanup (OstreeRepoAutoLock *lock);
-
 _OSTREE_PUBLIC
 const gchar * ostree_repo_get_collection_id (OstreeRepo   *self);
 _OSTREE_PUBLIC

diff --git a/tests/test-concurrency.py b/tests/test-concurrency.py
index 3ec3681c9e93b53d5ca89f99af4f738ab9f4bd62..e4ce21e9e1c77d81a68fc892971fc19c166ed73d 100755 (executable)
--- a/tests/test-concurrency.py
+++ b/tests/test-concurrency.py
@@ -44,7 +44,6 @@ subprocess.check_call(['ostree', '--repo=repo', 'init', '--mode=bare'])
 # and we don't need xattr coverage for this
 with open('repo/config', 'a') as f:
     f.write('disable-xattrs=true\n')
-    f.write('locking=true\n')
 
 def commit(v):
     tdir='tree{}'.format(v)